userdel: fix user busy detection for threads#1623
userdel: fix user busy detection for threads#1623haxtibal wants to merge 4 commits intoshadow-maint:masterfrom
Conversation
|
Reproducer: setuid.py #!/usr/bin/env python3
import ctypes, os, pwd, sys, threading, time
SYS_setuid = 105 # setuid, x86_64
libc = ctypes.CDLL(None)
libc.syscall.restype = ctypes.c_long
libc.syscall.argtypes = [ctypes.c_long, ctypes.c_long]
uid = pwd.getpwnam(sys.argv[1]).pw_uid
def thread_func():
libc.syscall(SYS_setuid, uid)
while True:
print(f"thread running as uid {uid} (pid={os.getpid()})", flush=True)
time.sleep(5)
threading.Thread(target=thread_func, daemon=True).start()
time.sleep(60)Start snippet as root. It's main thread has uid 0, spawned thread has uid of testuser. Then delete user while program is running. |
Could you please use only C and sh(1) (or bash(1)) for the reproducer? Python is hard to understand to me. |
6d51bbe to
a4681d8
Compare
Sure, try this: setuid_thread.c #include <pthread.h>
#include <pwd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/syscall.h>
#include <unistd.h>
static uid_t target_uid;
static void *user_thread(void *arg)
{
syscall(SYS_setuid, (long)target_uid);
for (;;) {
printf("thread running as uid %d (pid=%d)\n", (int)target_uid,
(int)getpid());
sleep(5);
}
return NULL;
}
int main(int argc, char *argv[])
{
if (argc < 2) {
fprintf(stderr, "Usage: %s <username>\n", argv[0]);
return 1;
}
struct passwd *pw = getpwnam(argv[1]);
if (!pw) {
fprintf(stderr, "user not found: %s\n", argv[1]);
return 1;
}
target_uid = pw->pw_uid;
pthread_t tid;
pthread_create(&tid, NULL, user_thread, NULL);
sleep(60);
return 0;
}Then gcc setuid_thread.c -o setuid_thread
sudo useradd --no-create-home testuser
sudo ./setuid_thread testuser &
sudo userdel testuser # should detect thread running as testuser and abort, but doesn't detect and deletes testuser |
Thanks!
Is the raw system call needed, or can we call the glibc wrapper? setuid(target_uid);From what you said, I think the raw syscall is necessary, but just to confirm.
|
Yes, see man 2 setuid:
To my knowledge there's no other way than raw syscalls to get that Linux specific feature of different thread UIDs within one process. |
Thanks for conforming! |
Could you please include this program (and the corresponding shell session below it) in the commit message? Please indent it, so that the includes are not treated as comments. |
On Linux, userdel/usermod check all /proc/<pid> status files to ensure a
to-be-modified user has no more running tasks, or abort modification
otherwise.
However, the check failed to detect threads running as the user if the
corresponding main thread ran as a different user. The user is deleted
despite still being busy. This is due to passing a wrong value to
check_status. The caller passed "<pid>/task", rather than
"<pid>/task/<tid>". In consequence check_status tried to open
"/proc/<pid>/task/status" - a wrong path that never exists - open fails,
and check_status always returns 0. The correct status file name would
have been "/proc/<pid>/task/<tid>/status" instead.
The bug can only be reproduced by rather exotic code using raw syscalls.
POSIX does not allow threads to have different UIDs.
To fix it, construct the correct path to the tid status file in the
caller, before passing it to check_status.
Reproducer:
// setuid_thread.c
#include <pthread.h>
#include <pwd.h>
#include <stdio.h>
#include <stdlib.h>
#include <sys/syscall.h>
#include <unistd.h>
static uid_t target_uid;
static void *user_thread(void *arg)
{
syscall(SYS_setuid, (long)target_uid);
for (;;) {
printf("thread running as uid %d (pid=%d)\n", (int)target_uid,
(int)getpid());
sleep(5);
}
return NULL;
}
int main(int argc, char *argv[])
{
if (argc < 2) {
fprintf(stderr, "Usage: %s <username>\n", argv[0]);
return 1;
}
struct passwd *pw = getpwnam(argv[1]);
if (!pw) {
fprintf(stderr, "user not found: %s\n", argv[1]);
return 1;
}
target_uid = pw->pw_uid;
pthread_t tid;
pthread_create(&tid, NULL, user_thread, NULL);
sleep(60);
return 0;
}
Execute in a shell
gcc setuid_thread.c -o setuid_thread
sudo useradd --no-create-home testuser
sudo ./setuid_thread testuser &
sudo userdel testuser
Behavior without fix:
No output, testuser is deleted.
Behavior with fix:
Output "userdel: user testuser is currently used by process 178863".
testuser is not deleted.
Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
a4681d8 to
31e8f42
Compare
|
Thanks! It looks good. I'll test it tomorrow. |
Change the interface of check_status and different_namespace to take pid and tid instead of a partially constructed path string. This is simpler and counters bugs like in the previous commit by design. Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
This protects against undefined behavior from wrongly used conversion specifiers. Note: snprintf unit test intentionally uses an empty format string to test, well, the empt format string. Thus override format-zero-length for it. Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de> fix format error
Also change previously existing linux specific format for pid_t to %d. Signed-off-by: Tobias Deiminger <tobias.deiminger@linutronix.de>
31e8f42 to
3d1b219
Compare
On Linux, userdel/usermod check all /proc/ status files to ensure a to-be-modified user has no more running tasks, or abort modification otherwise.
However, the check failed to detect threads running as the user if the corresponding main thread ran as a different user. The user is deleted despite still being busy. This is due to passing a wrong value to check_status. The caller passed "/task", rather than "/task/". In consequence check_status tried to open "/proc//task/status" - a wrong path that never exists - open fails, and check_status always returns 0. The correct status file name would have been "/proc//task//status" instead.
The bug can only be reproduced by rather exotic code using raw syscalls. POSIX does not allow threads to have different UIDs.
To fix it, construct the correct path to the tid status file. Also change the interface of check_status and different_namespace to take pid and tid instead of a partially constructed path string. This is simpler and makes similar bugs less likely.
Behavior without fix:
userdel testuser # testuser has threads with tid uid != pid uid=> no output, testuser deleted despite being busy
With the fix:
=> testuser detected as busy, not deleted